Repository navigation
Fix aggregate entity query return type. - #1063
travis-bradbury wants to merge 1 commit into
Conversation
|
The lint job failed due to apparently new things in phpstan, not things introduced in this PR.
|
mglaman
left a comment
mglaman
left a comment
There was a problem hiding this comment.
Thanks for this. The array_column() false positive is real and tracking aggregate queries on EntityQueryType is the right approach. A few changes below before merging.
#1064 and #1065 are on main now, so merging main into this branch should clear the lint failure.
Not blocking: \Drupal::entityQueryAggregate() still isn't covered, because DrupalStaticEntityQueryDynamicReturnTypeExtension doesn't handle it. That can be a follow-up. #669 is related.
| if ($varType->isAggregate()) { | ||
| // An aggregate query returns a list of result rows, each keyed | ||
| // by the aggregate results and the groupBy columns. | ||
| $rowType = new ArrayType(new StringType(), new StringType()); |
There was a problem hiding this comment.
Row values can be NULL: SUM() over only NULL values, or groupBy() on a field some entities don't have, since the field is added with a LEFT join. Other values are strings because the core MySQL, PostgreSQL, and SQLite drivers all set PDO::ATTR_STRINGIFY_FETCHES.
$rowType = new ArrayType(new StringType(), TypeCombinator::addNull(new StringType()));The fixture's expected types need to change to match.
There was a problem hiding this comment.
The values are now expected to be string|null instead of string.
| // by the aggregate results and the groupBy columns. | ||
| $rowType = new ArrayType(new StringType(), new StringType()); | ||
| return $varType->hasAccessCheck() | ||
| ? new ArrayType(new IntegerType(), $rowType) |
There was a problem hiding this comment.
nit: QueryAggregate::result() builds the rows with $return[] =, so this is really a list. The existing branches use a plain ArrayType too, so it's fine to leave this as is.
There was a problem hiding this comment.
I left this as is.
d876d0f to
3e7d57a
Compare
|
"Tests / Drupal core HEAD baseline check (pull_request)" failed. https://github.com/mglaman/phpstan-drupal/actions/runs/42102801854/job/127511254161?pr=1063. There's more fails in there than a previous merged PR (https://github.com/mglaman/phpstan-drupal/actions/runs/42082179892/job/127439501184) but they ran on the same drupal/core commit (7fa85d2). The "Ignored error pattern loadIncludes.fileNotLoadable" error is an existing issue — it happened in the other branch too. The other three, |
This changes the entity query extension so aggregate queries get the return type
array<int, array<string, string|null>>, instead oflist<string>. Aggregate queries give arrays whose keys are the aggregated columns.I added a fixture that includes the pattern I happened to see this with: an aggregate query wrapped in
array_column(), then using that in a condition. Phpstan incorrectly reported that's it's always false, becausearray_column('foo', $array)when$arrayislist<string>would always be an empty array.